Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CSS 6730 - add ofga model tests #1243

Merged
merged 10 commits into from
Jun 21, 2024
Merged

CSS 6730 - add ofga model tests #1243

merged 10 commits into from
Jun 21, 2024

Conversation

pkulik0
Copy link
Contributor

@pkulik0 pkulik0 commented Jun 20, 2024

Description

This PR includes tests that fully cover JIMM's authorisation model and ensure no breaking changes are introduced. Since OpenFGA doesn't have a procedure to alter existing models we need to verify the model behaves as expected, and that future changes to it don't break the intended behavior. It also adds the tests as a CI step.

Jira issue: CSS-6730

Engineering checklist

  • Documentation updated
  • Covered by unit tests
  • Covered by integration tests

@pkulik0 pkulik0 requested a review from a team as a code owner June 20, 2024 10:34
openfga/tests.fga.yaml Outdated Show resolved Hide resolved
openfga/tests.fga.yaml Outdated Show resolved Hide resolved
@pkulik0 pkulik0 requested a review from ale8k June 20, 2024 11:07
openfga/tests.fga.yaml Outdated Show resolved Hide resolved
Copy link
Collaborator

@alesstimec alesstimec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice.. a few comments, but generally lgtm

Copy link
Member

@babakks babakks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this. But can you please put the instructions on how to use the CLI at the top of the YAML file.

openfga/tests.fga.yaml Outdated Show resolved Hide resolved
openfga/tests.fga.yaml Outdated Show resolved Hide resolved
Comment on lines 191 to 194
- model:mo-1
- model:mo-2
writer:
- model:mo-1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mo-1 is listed in both of these, but I see we only explicitly grant user:mo-1 admin access to model:mo-1. So I'm surprised it appears under both in list_objects. I need to go read the docs on what list_objects is doing

Copy link
Contributor Author

@pkulik0 pkulik0 Jun 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's because of this:

- user: user:*
  relation: administrator
  object: model:mo-2

list_objects checks whether the relations you've listed fully match those of a given object, that's why you need to list models accessible by everyone too.

@pkulik0 pkulik0 merged commit d77ce65 into canonical:v3 Jun 21, 2024
4 checks passed
@pkulik0 pkulik0 deleted the CSS-6730-ofga-tests branch June 21, 2024 17:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants